Tournament description improvements #3074
Conversation
- Added logic to truncate titles with BBCode in both JS and PHP - Added basic infrastructure for testing the tournament UI - Added basic unit test of tournament description UI - Replaced Array.prototype.findIndex with more compatible Array.prototype.some
blackshadowshade
force-pushed
the
3068_tourn_description_markup
branch
from
334adfe to
d3d1682
Compare
cgolubi1
left a comment
cgolubi1
left a comment
There was a problem hiding this comment.
A few off the cuff comments; haven't looked at all of this yet.
src/engine/BMInterfaceTournament.php
Outdated
| * | ||
| * @param string $description | ||
| */ | ||
| protected function truncate_tournament_description($description) { |
There was a problem hiding this comment.
Remind me why we have to do it this way rather than reducing TOURNAMENT_DESCRIPTION_MAX_LENGTH in ApiSpec so people can't submit longer descriptions to the API in the first place.
There was a problem hiding this comment.
The major issue here is BBCode, which makes the input string appear longer than it actually is.
We could add the logic to strip potential BBCode to the validation code in the API if you prefer, but it's not as simple as just reducing TOURNAMENT_DESCRIPTION_MAX_LENGTH.
There was a problem hiding this comment.
To be clear, I'm trying to maximise the tournament description length that we allow. We could certainly reduce the allowed description length by 20 characters (the amount needed for ", Tournament Round 1"), but I think we can do better in the case where we actually aren't already at max length, which occurs when invisible BBCode elements are included in the description.
There was a problem hiding this comment.
The major issue here is BBCode, which makes the input string appear longer than it actually is.
What does "longer than it actually is" mean? Actually is for what function?
As far as i know, the limit that matters is the database field size. And what goes in the database is exactly the input with the unexpanded/unmodified BB code.
Is there a second place where we need to impose a limit?
There was a problem hiding this comment.
Imagine we have an inputted string that includes some BBCode, e.g., "[forum=1331,32731]Description[/forum]".
The input string to tournament generation is limited in its length by the textarea maxlength, which is TOURNAMENT_DESCRIPTION_MAX_LENGTH. That's as expected.
When we autogenerate a new game, we need at least 20 extra characters to add the string ", Tournament Round N" to the new game description, otherwise the database field size for the game description will truncate the game description unexpectedly.
One way to do this would be to restrict tournament descriptions to TOURNAMENT_DESCRIPTION_MAX_LENGTH - 20 or less.
A less restrictive way to do this is to strip all the BBCode when autogenerating the game title. In the above example, we would gain 25 characters by stripping the forum tags, which is more than enough to account for the extra characters necessary to show the tournament round number.
There was a problem hiding this comment.
When we autogenerate a new game, we need at least 20 extra characters to add the string ", Tournament Round N" to the new game description
I think reducing TOURNAMENT_DESCRIPTION_MAX_LENGTH by however many characters you need for the game description solves this problem exactly. The problem is:
- Because you're appending 20 characters (or whatever the actual number is --- point is it's a knowable number if we assume we're never going to have more than 1 billion tournaments or so), you need N characters for the tournament description database field and N+20 characters for the game description database field.
- Right now, we allow the same number of characters for both
So if we allowed 20 fewer characters for tournaments, that would actually solve this problem.
There was a problem hiding this comment.
I'll note that #3068 specifically calls out the fact that there is already a problem with the number of characters that we allow in the description being restrictive.
http://www.buttonweavers.com/ui/forum.html#!threadId=1258&postId=32256 also notes the felt restrictive nature of the description length.
While supporting BBCode allows for pretty much unlimited description, making the tournament description length even more restrictive than it has to be feels counterproductive to me.
There was a problem hiding this comment.
So:
- If we need to use the entire tournament description in the game description, then the tournament description "has to be" short enough to fit in the game description. The N-20 requirement already exists, we just have a bug in which we're not enforcing it.
- You're asserting that the first N-20 characters of the tournament description, arbitrarily truncated, is "good enough" for the game description. Basically you're saying there is no requirement that the player be able to anticipate what fraction of the tournament description will be used in the game description. I'm not sure i agree with that. [Edit: wordsmithed a bit, because it's not so much that i think the player needs the whole tournament description in the game description, as that i think the player needs to be able to reasonably predict what is going to happen with the game description based on the tournament description.]
At the risk of making more work: it seems to me that we're missing a field, tournament name. If a tournament had a (short, say, up to 100 chars) name and an (allowed to be longer) description, you could construct game description using the tournament name (and also show it in tables and stuff, and make our lives easier by not letting people put BBCode in it), and we could make the tournament description as long as everyone wanted and go home happy.
There was a problem hiding this comment.
Alternate idea: change the way we're constructing game descriptions so that "Tournament round M: " is the prefix of the game description rather than the suffix. Then what shows up in the table will be very unlikely to have BBCode, so our solution there won't matter, and if the long description is truncated oddly, that won't matter as much either because it won't be getting appended with another fixed string that looks weird.
src/ui/js/Overview.js
Outdated
| var descriptionNoMarkup = | ||
| Env.applyBbCodeToHtml(description).replace(/<[^>]+>/g, ''); |
There was a problem hiding this comment.
This seems inelegant - we're inserting all the HTML elements into this string we're going to truncate, and just kinda hoping we deleted them correctly.
We've had the original code in the Overview table for games for a long time. Why the urgency to change it now?
The right way to fix this is to make a variant of applyBbCodeToHtml() which removes the BB code, leaving the text, and use that to get your string. If you don't want to do that now, let's leave it as-is now, and do that later.
There was a problem hiding this comment.
I'm happy to add a new function that strips the BBCode out.
The urgency here is that this pull request will result in autogenerated game descriptions that are likely to have BBCode in them, so if we don't deal with this issue now, broken and truncated HTML tags will show up with high frequency in plain-text in the Overview table.
…riptions on the Overview page
| descText = description.substring(0, 30) + | ||
| ((description.length > 30) ? '...' : ''); | ||
| if (typeof(description) === 'string') { | ||
| var descriptionNoMarkup = Env.removeBbCodeFromHtml(description); |
There was a problem hiding this comment.
My understanding is that this diff is changing the behavior of the GAME table on the Overview page.
Is my understanding correct, or does this diff have something to do with tournament behavior?
There was a problem hiding this comment.
Your understanding is correct, we would be stripping all BBCode from descriptions of games on the Overview page.
There was a problem hiding this comment.
The stripping is to avoid the problem where the Overview shows something like this:
[forum=1331,32731]Description ...
instead of
Description with extra words
There was a problem hiding this comment.
Your understanding is correct, we would be stripping all BBCode from descriptions of games on the Overview page.
Okay. I have two points here, which are basically about documenting the behavior changes in PRs to make them easier to review:
- I know this PR is WIP, but the description doesn't say word one about changing the behavior of the games table. The description of a PR should describe the player-visible changes that appear in the PR. The reason it takes me so long to review these PRs is that i (i think correctly) assume there are a ton of player-visible changes which are not called out in the description, so i have to read the code and reverse-engineer them all before having an opinion about whether they are good. The more the PR description says what the intended player-visible changes are, the more we can skip to the second part.
- The games table has worked this way for a long time. It's not a new problem that's being introduced by allowing BBCode in tournament descriptions. That's why i'm questioning the urgency of fixing what shows up in these overview tables --- because afaik it's not a new problem. If it's something people have complained about, that's great evidence to put in the description of the issue being resolved.
There was a problem hiding this comment.
Re 1.: I had thought that by doing things the way I am, I was attempting to preserve the status quo, namely that the Overview page shows the description of the game, instead of showing a whole pile of BBCode that is obscuring the description of the game. I had not considered the fact that the change would be player-visible in the case when there was BBCode in the game title because I have never seen a game description with BBCode in any of my games.
Re 2.: I would argue that this pull request without this adaptation to the Overview page is actually creating a new player-visible problem because I have never seen a game description with BBCode before this pull request, but I am likely to see one after this pull request.
|
Also i should note that you said this PR is incomplete, so on the one hand i shouldn't kibbitz too much and penalise you for putting up an in-progress PR. OTOH, i want to make sure we're leaving open a way for people to review whether what you're doing is the correct design to solve the problems you're trying to solve. It seems like we could have that conversation:
I'll leave it to you to say which of those you prefer for this PR. |
|
Let me tell you how I normally put up a pull request when the code is finished, and why I'm currently working on an incomplete pull request. This may give you a better understanding of why there is no more detailed description at this point. Generally, I will be relatively detailed about changes that I have made in commit messages. In this particular case, I appear to have overlooked commenting the specific change about the Overview page, which is an oversight that dates from a few months ago, when I was trying to do some development in a small time window. If I am remembering correctly, I was being pressured by my wife to get off the computer because she wanted to go to bed, which resulted in me putting together a commit message as quickly as possible. When I normally create a pull request, I look through the changes that I have made, both via the commit messages and the code comparison, and summarise major changes that have been made as well as potentially unresolved issues that I have seen in testing. In this particular case, I set up a pull request because I was trying to remember what I was working on and thinking about back in September. This means that I haven't yet done the line-by-line scan of which changes I had made so that I could mention what I had changed. My major focus so far in the last few days (after getting CircleCI passing) has been to add QUnit tests for all the JS changes so that (i) we have some automated testing instead of stubs, and (ii) I can remember what behaviour I was aiming for. This is accompanied by actual manual testing to see what was working and what still wasn't, so I'm still in the phase of finding out what is broken and what could be a reasonable solution. This is why it's difficult for me to have already formulated a proposed solution: because I'm still at the stage of working out what breaks when we add support for BBCode in tournament descriptions. One sign for this is the use of magic numbers (e.g. 250 instead of TOURNAMENT_DESCRIPTION_MAX_LENGTH - N) in the code, which are there because I hadn't yet worked out what the actual limits should be. These will be replaced once the implementation settles enough that I actually know how many characters will definitely be there. I have found that having a conversation about UI design often devolves to "Show us what it could look like, and we'll iterate on that." I was attempting to get to a first cut (definitely not a completed product), so that we could then start a conversation that is based on actual screenshots and potentially a testing site. It seems that your experience is different, that you have experienced productive UI design conversations without basic functional visual elements. In my experience, I find that such a conversation turns out to be unproductive and prone to heading off on tangents, partly because parties talk past each other with words that the other party doesn't understand because they imagine things looking and behaving differently. I think that the ideal solution is probably a combination of A and B, so that people have something to look at and that there is a text description of what has been changed. As to the why things have been done the way they have, there are many layers to that and in my experience, this would be part of the conversation that follows after people have seen what you have done. People will ask why questions that you never considered or will be interested in a nuance that you need to consider from multiple directions, this is what helps to improve the final design. I am not unwilling to put my current approach into words, I just think that it will receive as much feedback as we currently have received on #3068 when I detailed the two difficulties I was trying to tackle. |
|
Yeah, that's fair enough --- it's a good callout that i should be better about participating in conversations about strategy when those conversations happen on issues rather than PRs. |
|
I've padded out the initial description of this pull request. It is still incomplete because I'm intending to add a few more QUnit tests, but I thought I'd get testers involved in the conversation now on the two issues that Chaos highlighted: (i) How should we deal with BBCode in autogenerated game descriptions when we are looking at the game or tournament overview pages? In the current version of this pull request, I'm strip all BBCode from all game descriptions (including non-tournament games) before we truncate the description to 30 characters. @cgolubi1 has suggested maybe using a workaround of only showing a prefix something like "Tournament round M:" followed by only a few characters of the game description, but I'm not convinced that this is a reasonable solution. (ii) How should we deal with long tournament descriptions that are at or near max database field length (255 characters) when auto-generating tournament game descriptions that end with the qualifier ", Tournament Round N"? In the current version of this pull request, if the tournament description is longer than 235 characters, I strip BBCode if possible and then if it is still longer than 235 characters, truncate it to 231 characters and add an ellipsis before adding the tournament round qualifier. @cgolubi1 and @blinkingline have suggested a blanket reduction of the character restriction in the input field to 235 characters as an alternative. |
|
For Single Elimination tournaments, ", Tournament Round 1" is enough characters for the auto-generated game text. However, in future tournament types we may need more text in the game description to describe what is happening. I'm not a fan of limiting tournament descriptions to 235 characters. My preference is that the game description be simple. "Tournament ## Round 1" is all that's needed. We don't need to copy the tournament text. We have a link to the tournament on the game page which has the longer description. (In fact, the game description "Tournament ##" could be a link to the tournament page.) Another possibility (more work) is that a tournament has a title along with a description. The title can be short (100 characters?) and it's the title along with ", Tournament Round 1" which is used in the game description. Could the tournament text allow BBCode and not be limited in length, like a forum post? That would be the most ideal situation. I know that we can include a long description of the tournament in a forum post ... but we have no way to link from a tournament page to a forum post. |
|
"Could the tournament text allow BBCode and not be limited in length, like a forum post? That would be the most ideal situation. I know that we can include a long description of the tournament in a forum post ... but we have no way to link from a tournament page to a forum post." After thinking about it today, I realize that if the tournament description allows BBCode that will be sufficient to create a link to a longer forum post. I thus don't see a need for the size of the tournament description to be unlimited. |
…urnament descriptions - Replaced magic numbers with an actual calculation of the maximum allowable length
|
(I fixed my previous post - I meant that if a tournament description can link to a forum post then there's no need for it to be unlimited in length.) |
My first instinct was to agree: I think the tournament should have a description, and games in the tournament should link to the tournament, and don't need to copy the description. If the description says how to choose buttons and stuff, that doesn't need to be in the game; and if there's a link to a forum post, I think it's fine if that isn't in the game description either, players can find it by clicking through the tournament page. But, I thought of a possible situation where it would be good to have some details in the game: Tournaments with rules that apply to the game, and which players might need to remember while they're playing the game. Another more complicated proposal: What if the tournament had both a tournament description (which would only be on the tournament page) and a game description (which would be used as the description of each game in the tournament)? Then tournament creators could put tournament-relevant info in the former, and game-relevant info in the latter. More generally, I don't like the idea of stripping out BBCode, at all, under any circumstances, and think we should design things so that it's not necessary to do that, rather than saying "well, in this context the BBCode probably won't ever be very important". |
|
I'm not sure I've followed all the details, but my general impression is that blackshadowshade's short term solution seems reasonable for now, with the one caveat that having different behavior for the API seems like technical debt (IIUC what's being proposed). Longer term, the request to have a separate title field with a fairly tight length restriction and without BBCode does seem like a good way to reserve some extra length for the description field. |
|
At https://www.buttonweavers.com/ui/forum.html#!threadId=1336&postId=32820, tasha suggested that we might consider truncating based on punctuation, which is often used between the tournament title and the tournament description. |
|
To elaborate on my previous comment: I don't think we should be truncating anything either; it just seems too likely to lead to trouble of one kind or another. Can't we design an implementation that doesn't require any sort of truncation at any point? |
|
We almost always truncate the description on the Overview screen. Are you suggesting that we have a description for auto-generated games that is shorter than the 30 characters allowed there? Also, for any games (not just auto-generated) with BBCode in their description, are you happy with plain-text BBCode fragments showing up in the description field of the Overview table? This happens now if the closing tag is deleted by truncation. |
|
Well, so I guess there's two things:
I don't have a strong feeling about exposing BBCode; I'm not sure that truncating "Make sure not to [b]assume[/b] anything in this game!" to "Make sure not to [b] ..." is any better or worse than truncating it to "Make sure not to ass ..." for example. |
|
I'm all for using thoughts about greenfield design to help us orient ourselves to where we actually want to end up. Regarding 2., the point is that very few (perhaps none) of the game descriptions that I have seen contain BBCode, so I personally haven't had the BBCode truncation issue show up. However, if we maintain the status quo of autogenerating game descriptions from tournament descriptions and the tournament description starts with a forum link, we'll end up with something like Perhaps this isn't a big deal, maybe the description field isn't actually that useful on the overview screen, but the displayed text now has an obvious mismatch with the actual tournament description because normally-invisible BBCode is now visible. |
|
I think y'all are batting around ideas about the overview table, so i'll leave you to that for now. The other topic we've been discussing is the construction of game descriptions when the tournament description is long enough that adding the mandatory suffix takes it over 255 characters. For that one, i want to ask the question: what would we have to do in order to never have to truncate text in that case? Isn't it just "make the game description field in the DB N characters larger" for some calculable value of N? That doesn't sound so hard. Should we consider doing that? |
|
If we agreed that N was calculable right now and that we'd be happy to increase N if the requirement increased, then yes, this solution would work. |
|
Alternatively, if we increased the game description field in the database by a large number so that we are highly unlikely to reach the limit, that would also work. |
|
What do folks think about the idea of tournaments having separate fields in the tournament object for Tournament Description and Game Description? More trouble than it's worth? I think the user experience would be nice, especially if the Game Description could default to the Tournament Description on the tournament creation page, so tournament creators wouldn't have to fill anything in if they didn't want to. |
Yeah, sounds good. We don't have to pick this option (i certainly don't object to some sort of tournament metadata split, as i, irilyth, and others, have suggested in various places), but i think we're agreed that increasing the DB field size for games wouldn't be very hard to do. So we shouldn't do anything very complicated or error-prone to avoid it. |
|
I think that resolves the issue about auto-generated game descriptions for the game page: no more truncation there, instead, we just increase the database field size by 50, accounting for possible future suffixes like ", Tournament Round 2, Winners' Bracket". My intention would be to move the responsibility for such suffixes to the BMTournament* class. |
|
Note that this doesn't prescribe us changing our design to split things into a title and description for the future, it just resolves the issue within the scope of this pull request. That leaves us to sort out what happens on the overview page regarding truncation and BBCode. |
- Made game description field 50 characters longer in database to account for autogenerated tournament suffix
blackshadowshade
force-pushed
the
3068_tourn_description_markup
branch
from
d7a4bcb to
b2961af
Compare
|
I have removed all truncation and BBCode stripping from the backend, and increased the database field size for game descriptions from 255 to 305. That means that any modification to the tournament description (as present in the autogenerated game description) on the Overview page is Javascript only, the API provides the whole thing. Remaining QUnit tests to add are on test_TournamentOverview.js, where we have nothing but stubs. |
5 participants
Fixes #3068. Fixes #3071.
With this pull request, we now:
In this pull request, I have:
To do:
I note that the concept of separating the "title" and "description" of a tournament has been mentioned multiple times now (http://www.buttonweavers.com/ui/forum.html#!threadid=1258&postId=32257 and #3074 (comment)). Implementation of this idea falls outside the scope of this pull request but could definitely be a new issue if we want to pursue it, and discussion of the idea may certainly be pertinent for the changes in this pull request.